feat(status): expose row-copy progress as a structured field on Progress by aparajon · Pull Request #1220 · block/spirit

aparajon · 2026-09-06T21:59:03Z

Why

status.Progress already carries the ETA (ETA), the checksum counters (Checksum), the throttle state, and per-table row counts, but the runner-wide row-copy progress only reached callers inside Summary, as text: 1031251/16370180 6.30% copyRows ETA 5m. A wrapper that wanted those two numbers had to parse the string back out, which is the one thing Summary was never meant for. The numeric type already exists (status.CopyProgress is what the copier's CopyProgress() returns for the periodic status block); it just was not on Progress.

What

Adds Copy status.CopyProgress to status.Progress, next to Checksum. It is the sum of Tables, so the two reconcile by construction: both count settled rows against the tables' cardinality estimates. It is populated as soon as the copy chunker exists and keeps its final reading through the later phases, so a caller can read how much the run copied at any point. RowsTotal is an estimate, so RowsCopied can exceed it, exactly as it already can per table.
It is deliberately not the copier's own CopyProgress(). On the chunker Spirit selects for a single auto_increment key, that measures keyspace distance against the auto_increment max rather than rows, and summing it across a multi-table run adds an id to a row count. A MySQL-backed test on that path (TestProgressCopyReconcilesWithTablesOnAutoIncrementKey) pins Copy against Tables on a table whose ids are sparse, where the copier's own measure differs by orders of magnitude.
The periodic status block's copier row is derived from the same chunker snapshot, so the log line and the API report one measure on the same tick. Each runner reads the chunker once per call through a small copyTables helper (migrate, move) or its existing snapshot (sync).
The migrate, move, and sync runners render Summary from that same reading and from a single GetETAState() call, so the copy fraction, the ETA text, and the ETA field describe one instant and a poll takes the copier lock once instead of three times. status.ETA gains a String() for this, and the copier's GetETA() now delegates to it.
Behavior change in Summary and the status log block: on an auto_increment key the copy fraction now reports rows, the same numbers as Tables, instead of keyspace distance. On a table whose ids are sparse after years of deletes, the old text could read 0.50% with half the rows copied. The ETA is unchanged: it is still derived from the copier's keyspace pacing, which is the right basis for time remaining. The format of the line is the same, but anything rendering the percentage downstream sees it change meaning with this release, and on a sparse table the percentage can now visibly lead the ETA.
The optimistic chunker's checkpoint watermark now carries the settled row count beside the resume position, in the same {ChunkJSON, RowsCopied} envelope the composite chunker already writes, and OpenAtWatermark restores it. Without that, a resumed copy on an auto_increment key would have reported its rows from zero where the old keyspace measure resumed at the right place. Bare chunk watermarks from checkpoints written before this change are still accepted and resume with a zero count, and the shared watermark parsers (WatermarkPerTable, the move recopy clause) already unwrap the envelope. TestCheckpoint asserts the count across the checkpoint cycle, including that re-copying rows already present settles nothing new. The copy aggregate each runner reports to its metrics sink stays per invocation: the count restored at resume is recorded and subtracted when the copy completes, so rows and chunks in that callback remain commensurable. A move deletes and re-copies the rows at or above the resume position, so the previously settled rows among them are counted again in Copy; that is documented on the field and left for a follow-up, since an exact correction needs the chunker to learn how many rows the recopy delete removed.
Two caveats are documented on the field rather than hidden: RowsCopied counts rows the copy settled, so rows the binlog applier wrote ahead of the copy are not counted (INSERT IGNORE reports them as unaffected) and a busy table finishes short of its estimate; and on an auto_increment key the ETA (including DUE) stays paced on the keyspace, so the two halves of Summary can disagree on how close a copy over a sparse key range is. The status and copier READMEs and the migrate guide are updated to match.
A shared copiertest.Stub replaces the three per-package copier stubs in the runner tests. It lives beside the copier rather than in testutils because the copier's own tests import testutils, so a stub there would be an import cycle.

With this, every value that appears in Summary has a typed counterpart on Progress, so a consumer never has a reason to parse it.

The copier's GetProgress() string is no longer called anywhere in Spirit outside its own implementation. It is left in place here because it is part of the exported copier.Copier interface; retiring it is a separate, breaking change.

Opened by Claude (Fable 5).

The runner-wide row-copy counts only reached callers inside Summary, as text, while the ETA and checksum counters already had typed fields. Add Copy (status.CopyProgress) to status.Progress, populated during CopyRows by the migrate, move, and sync runners, and render Summary from the same reading so the two cannot disagree within one snapshot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

morgo

🤖 Automated second-pass review on Morgan's behalf. CI is green at 8f5b9a28 and the mechanics are clean — all three Progress() producers are updated, every status.Progress literal in the tree is keyed so the new field breaks no construction, and GetProgress() is literally CopyProgress().String() (buffered.go:630), so Summary is byte-identical before and after. The stated goal is right, too: a consumer should never have to parse Summary.

Not approving yet, for one reason. The numbers being promoted are not row counts on Spirit's most common code path, and the new doc comment says they are.

table.NewChunker picks chunkerOptimistic for any table with a single auto_increment key (pkg/table/chunker.go:168) — the default for most production MySQL tables. That chunker's Progress() returns:

numerator t.rowsCopied, declared at chunker_optimistic.go:70 as // The sum of chunkSize: distance travelled, not a row count, and a different field from actualRowsCopied, which is what RowsCopied() returns
denominator maxValue — the auto_increment max value, not a row estimate

buffered.CopyProgress() passes both straight into status.CopyProgress{RowsCopied, RowsTotal}. Meanwhile Progress.Tables[] is built from CopyRowCounts(), which returns settled rows and Ti.EstimatedRows. So on a table with 1M live rows whose IDs are sparse to 100M, one snapshot reports Copy: 500000/100000000 (0.50%) and Tables[0]: 500000/1000000 (50%) — same struct, same field names, two orders of magnitude apart.

pkg/table/row_counts.go:8-9 already warns about precisely this: "Progress instead measures keyspace distance for optimistic chunkers, so its numerator and denominator must not be presented as literal row counts."

That warning was survivable while these numbers only existed inside a human-readable Summary. Naming them RowsCopied/RowsTotal on the public Progress struct, next to Tables[], is what invites a consumer to do arithmetic on them and to reconcile the two — which they can't. Two ways out, either fine by me:

source Copy from CopyRowCounts semantics so it reconciles with Tables[], or
keep the current source and name it for what it is (Position/Extent, or keep CopyProgress but document the provenance honestly).

Why this survived review: the only test pinning Copy next to Tables is TestE2EBinlogSubscribingCompositeKey, and a composite key routes to chunkerComposite — the one implementation where Progress() and RowsCopied() read the same field, so they cannot disagree. It asserts {1000,1200} for both and passes. The divergent default path has no coverage at all.

The remaining notes are inline and none of them block: a multi-table unit-mixing consequence of the same root cause, Copy zeroing at the phase boundary while Tables keeps the totals, three copier-lock acquisitions per call, a duplicated test stub, and a %v-on-Stringer nit.

Worth saying: everything else about the change is the right shape. Reading the copier once and rendering Summary from that same reading is a real improvement — it removes a genuine disagreement window — and placing Copy beside Checksum with matching doc structure is consistent with the existing API. The problem is one level down in what the copier hands you, not in this PR's structure.

…h Tables Copy was read from the copier's own progress, which on the chunker Spirit selects for a single auto_increment key measures keyspace distance against the auto_increment max rather than rows. Tables was already built from the row-count path, so the two fields could tell different stories in one snapshot, and Copy went back to zero the moment the copy phase ended. Copy is now the sum of Tables, built whenever the copy chunker exists, so it reconciles by construction and keeps its final reading through the later phases. Summary renders from the same reading and from one GetETAState call, with a new ETA.String that the copier's GetETA also uses, so a poll takes the copier lock once. The three runner packages share one Copier stub in copier/copiertest, and a MySQL-backed test on the auto_increment path pins Copy against Tables where the copier's own measure diverges. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Kiran01bm · 2026-09-07T00:07:06Z

🤖 Review findings - created by Kiran's code review agent - for spirit/pull/1220, 97fd3af.

Verdict: 8 findings — 0 blocking, 5 non-blocking (a resume-path undercount, two runners whose derivation is entirely unpinned, and a docs sweep the PR skipped), 3 suggestions. The reconciliation itself is right, and pkg/migration genuinely proves it.

Non-blocking

1. On resume, Copy.RowsCopied restarts from zero while Tables does not. CopyRowCounts reads chunkerOptimistic.RowsCopied(), i.e. actualRowsCopied — and OpenAtWatermark re-seeds only rowsCopied (the keyspace counter the pre-PR numerator used), leaving actualRowsCopied at 0. So a resumed migration reports Copy counting only post-resume rows against a full RowsTotal. The composite chunker is immune (chunker_composite.go:301), Resume: true ships in the same snapshot, and TableProgress.RowsCopied already documents the caveat — so this is a known-shaped gap, but Copy is the new headline number and inherits it silently.

2. The operator log block still renders the measure this PR declares wrong — on the same tick. migration/runner.go:1989 feeds Status() from r.copier.CopyProgress() (keyspace) while Progress().Summary now renders settled rows, and status/task.go:48,53 calls both on the same 30s tick. On the sparse-auto_increment table progress_copy_test.go builds, the API says 500/501 99.80% while the log line prints copier 0.20% 2000/1000000. Same pattern at move/runner.go:1547 and datasync/runner.go:1672; no test compares the two, since every Status() assertion is substring-only.

3. move and datasync would not notice their Copy reverting to the keyspace position. Swapping move/runner.go:1995 and datasync/runner.go:1607 back to chunker.Progress() passes both full suites — their Copy assertions pin only RowsTotal, with RowsCopied always 0. Only pkg/migration has a test that distinguishes the two measures. The fix needs no database: MockChunker keeps rowsCopied (fed by Feedback) separate from currentPosition (fed by SimulateProgress), so one Feedback(nil, 0, 50) per test makes them diverge.

4. Multi-table RowsCopied summation — the point of CopyFromTables — is unpinned everywhere. Summing only tables[0] at status/tables.go:37 passes the full pkg/migration (62s), pkg/move, pkg/datasync and pkg/status suites. An 8-table atomic migration would report ~12% in Copy forever while Tables reads 100% — exactly the reconciliation this PR exists to establish, silently broken with CI green.

5. The PR touches zero .md files, and five doc sites now describe the old measure. pkg/status/README.md:89 omits Copy from the Progress field list; :72 and docs/migrate.md:672 describe the keyspace measure in row-count language; :131 says the ETA is cleared after CopyRows while Copy now deliberately persists past it; and pkg/copier/README.md:61,170 still recommends GetProgress(), which this PR leaves with zero production callers.

General suggestions

6. Summary mixes a row-based fraction with a keyspace-gated DUE. The percentage now comes from settled rows, but DUE is still gated on the keyspace percentage at copier.go:34, so the string can read near 100% for hours without ever reaching DUE — and conversely show 80% beside ETA DUE. This is disclosed, deliberate design (two verifiers refused to call it a bug), but the two halves of one line answering different questions is worth a sentence in the field doc.

7. Both newly exported symbols landed without a test in their own package. CopyFromTables (+13) and ETA.String() (+25) live in pkg/status, which already has DB-free tables_test.go and progress_test.go, and neither was extended — gutting either survives go test ./pkg/status/.... Since every go test in this repo runs inside the Docker+MySQL matrix, a contributor running pkg/status locally gets a false green. Relatedly, copiertest.Stub's new Copy/Chunk fields are set by datasync's test but read by no assertion.

8. progress_copy_test.go:68 pins the chunker's internals rather than the property. require.Equal(status.CopyProgress{RowsCopied: 2000, RowsTotal: 1000000}, ...) couples the only test that distinguishes the two measures to the optimistic chunker's default first chunk size and open-lower-bound behaviour. The load-bearing assertion is p.Copy.RowsTotal < 1000000 on line 62; a chunker tweak turns line 68 red, and the natural "fix" is to update the constant — quietly disarming the test.

The one thing that could have broken, verified

The DUE/TBD switch was lifted out of copier.GetETA() into status.ETA.String(), while GetETA() itself is still called at three sites to render the human log block — a silent behaviour change there would print eta=0s through an entire copy. It holds: the rewrite is behaviour-identical including the ETANone fallthrough and the locking, and gutting ETA.String() to e.Duration.String() is caught by two pkg/migration tests. Worth noting the full pkg/copier suite (15s) passes that mutation, so the guarantee rests entirely on the MySQL-matrix job.

Verified correct

No test was weakened to hide a regression: binlog_test.go:165,180,211 now pin Copy by whole-struct equality, including a deliberate 1201/1200 100.08% over-100% case.
The require.Empty(p.Copy) → require.Equal({RowsTotal: 100}) flips in all three runners are the intended behaviour change (the reading outlives the copy phase), not a loosening.
No divide-by-zero or NaN: status.fraction() guards total == 0, so a zeroed RowsTotal renders 500/0 0.00%.
The Progress() sweep is symmetric across migration, move and datasync — no runner was left on the old derivation.
Deriving Copy from the copier's own measure instead of CopyFromTables is caught in both move and datasync, so the "not the copier's own measure" half of the contract is genuinely pinned.
No new data race; -race green across pkg/status, pkg/copier, pkg/migration, pkg/move, pkg/datasync.

This review was generated by Claude Code (claude-opus-5).

The periodic status block still rendered its copier row from the copier's own progress, so on an auto_increment key the same tick could log a keyspace fraction while Progress reported settled rows. Each runner now derives both from one snapshot of the copy chunker, through a copyTables helper in migrate and move and the existing progMu snapshot in sync. The mock-based runner tests now feed settled rows into the chunkers so that Copy.RowsCopied diverges from the copier's own measure, and the multi-table cases sum both counters, which pins what CopyFromTables exists for. The new status symbols get unit tests in their own package, the auto_increment test asserts the property rather than the chunker's default chunk size, and the field doc, status and copier READMEs, and the migrate guide describe the row measure, the resume caveat, and the keyspace-paced ETA beside it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

aparajon · 2026-09-07T01:05:13Z

🤖 Thanks, all eight taken in 8672498. The status block's copier row now comes from the same chunker snapshot as Progress, so the log and the API report one measure on the same tick. The move and datasync tests feed settled rows into the mock chunkers so a revert to the copier's measure fails, and the multi-table cases sum both counters. ETA.String and CopyFromTables have unit tests in pkg/status, and the auto_increment test asserts the property instead of the default chunk size. The docs sweep covers the field doc, both READMEs, and the migrate guide, including the resume caveat and the keyspace-paced DUE beside a row-based fraction.

On finding 1: the optimistic chunker has no persisted settled-row count to re-seed on resume, so I documented the gap alongside Resume rather than fabricate a number. Happy to take that as a follow-up.

Claude (Fable 5)

The copier row of the status block now counts settled rows against the table's row estimate rather than keyspace distance against the auto_increment max. The checkpoint test pinned the old figures literally; the estimate comes from table statistics and the seed leaves auto_increment gaps, so it now reads both from the database. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Kiran01bm · 2026-09-07T01:29:04Z

🤖 Review findings - created by Kiran's code review agent - for spirit/pull/1220, 515ae34.

Verdict: re-review of the delta 97fd3afe..515ae348 only — 8 findings, 0 blocking, 6 non-blocking, 2 suggestions. The switch from the copier's keyspace measure to the per-table settled counts is correct and now well covered in two of the three runners; the increment 86724981..515ae348 is a genuine flake fix, not a loosening.

Non-blocking

1. On an auto_increment resume the copier log row now restarts near 0% while eta= does not. RowsCopied() returns actualRowsCopied, which chunker_optimistic.go:692 zeroes in open() and OpenAtWatermark re-seeds only for t.rowsCopied (:371) — so post-PR the percentage and n/m restart from zero on a resumed run while the ETA still continues from the resumed keyspace position. This is log-only (nothing parses Status()) and the semantics are the pre-existing documented contract of Chunker.RowsCopied, so it is not a regression in behaviour — but it leaves one row of the block internally inconsistent on exactly the path the PR set out to make consistent. docs/migrate.md:672 carries no resume caveat and the log block has no Resume indicator though Progress does; composite chunkers are genuinely unaffected.

2. The move runner's share of the change is completely untested. Reverting pkg/move/runner.go:1539 back to r.copier.CopyProgress() — undoing the PR's entire purpose for that runner — leaves the whole pkg/move suite green, because move.Runner.Status() has zero test callers repo-wide. The same mutation dies immediately in migration (progress_copy_test.go:77) and datasync (progress_test.go:46); one require.Contains(t, r.Status(), "50/100") in TestMoveProgress closes it with no new fixture.

3. The new log-block assertions pin n/m but not the leading percentage. Sourcing the percentage from the copier while keeping the counts from the tables passes both new assertions; tightening datasync's to "77.78% 70/300 chunk-size=25 eta=1m0s" confirms that render is reachable — a keyspace-derived 77.78% printed next to 70/300 settled rows (23.33%). That is precisely the two-measures-in-one-row confusion this PR exists to remove, sitting three characters outside the assertion.

4. Chunker's godoc and the copier README still describe the old contract. pkg/copier/copier.go:68 still documents CopyProgress as the progress source; the README's interface block omits GetETAState, CopyProgress and ChunkSize while its Methods list documents all three, and its monitoring example calls CopyProgress() nine lines after warning against using it. After this PR CopyProgress() has no production callers inside spirit at all.

5. Datasync's cp != nil guard now gates figures that no longer come from the copier. pkg/datasync/runner.go:1671 reads the chunker for the copy figures but the surrounding nil-check is still on the copier, so a nil copier with a live chunker silently drops real progress. Reading the chunker under progMu is correct here — that is datasync's documented guard and it has no chunkerMu.

6. Datasync inlines the helper the other two runners extracted. Migration and move both grew copyTables(); datasync open-codes the same two lines. AGENTS.md's runner-triplet rule asks the three to stay shaped alike, and the asymmetry is what makes the cp != nil mismatch above easy to miss.

7. [increment] The resume test's denominator is now self-fulfilling. estimatedRows is read from the same TableInfo.EstimatedRows the production path prints via CopyRowCounts, so multiplying the estimate by 7 at its only write site (tableinfo.go:210) leaves TestCheckpoint green. The old literal was not a usable oracle either — see below — so this is a trade, not a regression; SELECT COUNT(*) FROM cpt1 would restore an independent bound. The stale comment at resume_test.go:95 claiming the fixture produces "exactly 11040 rows" should go.

General suggestions

8. Two test-strength nits in the increment. progress_copy_test.go:70 is now the only assertion on copier.CopyProgress() anywhere in the repo and bounds its numerator only from below — a 100× error survives; an exact require.EqualValues(t, 2000, own.RowsCopied) is deterministic here (2 chunks × the fixed 1000-row chunk size). Separately the require.Eventually at :212 computes wantCopier into a local, so a failure costs 10s and never prints the expected string — append it to the message.

The one thing that could have broken, verified

Redirecting the API's Copy figures from the copier to the summed per-table counts. Mutating each runner's Status() back to r.copier.CopyProgress() dies in migration and datasync; dropping the RowsCopied accumulation in CopyFromTables dies at five independent sites; dropping the chunkerMu guard in copyTables() dies in both runners under -race, which spirit's CI runs. Only move survives — finding 2.

Verified correct

The increment 86724981..515ae348 is test-only and a real flake fix: TestCheckpoint at 86724981 fails unmodified against mysql:8.0.45 (rendered 0/10546, then 0/11092, against the hardcoded 0/11040), because the denominator is InnoDB's sampled information_schema.table_rows. With the increment it passed 8/8.
The %6.2f width arithmetic in the new assertion reproduces production alignment at 1-, 2- and 3-digit percentages, including ≥100% (reachable, since RowsCopied can exceed RowsTotal).
The settled-rows numerator is a genuinely independent oracle — it queries _cpt1_new directly, and doubling the production numerator inside CopyRowCounts fails the assertion.
ETA.String()'s DUE short-circuit is properly pinned; the due_ignores_a_leftover_duration subtest is the only thing distinguishing it from a coincidentally-zero duration.
The earlier claim that the composite-chunker ETA docs are now wrong is refuted — both doc sites are explicitly scoped to an auto_increment / sparse-id key.
copiertest.Stub's move to Copy{7,9} is what makes three previously-surviving mutants die; the value is deliberately unmistakable against any plausible table sum.

This review was generated by Claude Code (claude-opus-5).

…er docs The datasync status block now follows the chunker rather than the copier, since the chunker is published a step earlier; until the copier exists the row reads chunk-size=0 and eta=TBD. The move and datasync tests assert the rendered copier row, percentage included, and the MySQL-backed migration test pins the copier's own numerator to two chunks of the configured size. The resume test bounds the row estimate against a real COUNT(*) so the denominator is no longer self-referential. The copier godoc and README now describe CopyProgress as the pacing measure it is, and point callers who want rows at status.CopyFromTables. The migrate guide notes that a resumed copy restarts its row count while the ETA continues. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

aparajon · 2026-09-07T01:50:33Z

🤖 Thanks for the second pass. Addressed in ce54a2c, per finding:

Resume restarts the copier row near 0% while the ETA continues — documented, not changed. The checkpoint persists the copy position, not a settled-row count, so re-seeding actualRowsCopied on resume would need a new persisted value. The n/m row in docs/migrate.md now states the behaviour; happy to take the re-seed as a follow-up if you want it.
move.Runner.Status() had no test caller — copiertest.Stub now reports a throttler.Noop, and TestMoveProgress asserts the rendered copier row (50.00% 50/100 chunk-size=0 eta=1m0s throttled=false) and that the stub's own figures do not appear.
Percentage not pinned — the datasync and migration log-block assertions now include the leading percentage.
Stale CopyProgress godoc and README — the godoc now describes it as the chunker's Progress (keyspace distance on the optimistic chunker, the measure the ETA is paced on) and points row-reporting callers at status.CopyFromTables; the README interface block lists GetETAState, GetProgress, CopyProgress, ChunkSize, and the monitoring example sums the chunker's tables instead of calling CopyProgress().
Datasync guard on cp gates chunker-derived figures — the row now renders whenever the chunker is published; chunk-size and ETA read 0 / TBD until the copier exists. A new assertion covers the tick between the two publications.
Datasync inlines what migrate/move put in copyTables() — declined. Datasync snapshots every published field under one progMu acquisition; a copyTables() helper would take the lock a second time. Kept inline with the snapshot.
Resume denominator self-fulfilling — the test now bounds EstimatedRows against SELECT COUNT(*) FROM cpt1 (InEpsilon 0.2), and the fixture comment no longer claims an exact row count.
Literal expectations — the copier's own numerator is asserted as 2*m.copier.ChunkSize(), and the resume test's Eventually failure message includes the expected row.

Claude (Fable 5)

Kiran01bm · 2026-09-07T02:08:06Z

🤖 Review findings - created by Kiran's code review agent - for spirit/pull/1220, ce54a2c.

Verdict: re-review of the delta 515ae348..ce54a2c2 only — 5 findings, 0 blocking, 2 non-blocking, 3 suggestions. Seven of the eight findings from the previous round are fixed and independently mutation-verified; the remaining items are one leftover asymmetry and two small doc gaps.

Non-blocking

1. The API and the log block now disagree in the new no-copier window. datasync/runner.go:1675 substitutes ETAMeasuring (renders TBD) when the chunker is published but the copier is not, while Progress() in the same window returns a zero ETA — state ETANone, documented as "there is no copy ETA because the migration is not in the row-copy phase", though the phase is CopyRows. Confirmed empirically: on one tick Status() renders eta=TBD while Progress() returns ETA{State:"", Duration:0}. ETAMeasuring is the right choice for the log block (ETANone renders 0s, which would falsely read as "no time remaining"), so the fix belongs on the Progress() side.

2. The resume caveat landed one row too low. The new sentence is in the n/m row, but docs/migrate.md:672 — the % row directly above, which a reader hits first — still says the percentage "can drift slightly", the opposite of what a resumed auto_increment copy does. Neither row states the sharper consequence: on a resume n/m never reaches 100%, since n counts only post-restart rows against the whole-table estimate.

General suggestions

3. Datasync still inlines the helper the other two runners extracted. datasync/runner.go:1673 (and again at :1602/:1609) open-codes status.CopyFromTables(status.TablesFromChunker(chunker)) while migration and move both have copyTables(). This was the one previous finding not addressed, and AGENTS.md's runner-triplet rule asks for the port or a shared extraction.

4. The nil-copier guard was added to one runner only. Migration and move dereference r.copier unconditionally in their CopyRows branches. I found no reachable window in either (both publish the copier before setting CopyRows), so this is consistency rather than a bug — but it is the one place the delta's own reasoning, "a status tick can land with either missing", was applied to a single runner.

5. The README refresh is half-done in its own file. ChunkSize() uint64 was added to the interface block at README.md:46 but the Methods bullet list below it still has no ChunkSize entry, so a reader looking up what chunk-size= means falls through to the Go source.

The one thing that could have broken, verified

Whether the new assertions actually kill what survived last round. They do, in every runner: sourcing the percentage from the copier while keeping the counts from the tables now dies in migration, datasync and move; reverting move's Status() to r.copier.CopyProgress() dies at progress_test.go:57; inflating the copier's own numerator 100× dies at progress_copy_test.go:72; reverting datasync's guard to cp != nil and weakening the stand-in ETA to ETANone both die too. Five mutants that survived at 515ae348 are now killed.

Verified correct

The 2*ChunkSize() pin is genuine, not tautological — ChunkSize() reads the sampled chunk size while RowsCopied comes from the chunker's Progress.
require.InEpsilon(actualRows, estimatedRows, 0.2) has real headroom: across 28 runs against a fresh mysql:8.0.45 the estimate took four values (11092/10810/10546/10264) against a true count of 11010 — worst error 6.8%, so ~3× margin. Argument order and uint64 handling are correct.
The old "exactly 11040 rows" comment was itself wrong (the seeds produce 11010), so replacing it was right.
The estimate is not re-read mid-test: AutoUpdateStatistics only starts from Run(), which this hand-stepped test never calls.
The caveat's scoping to auto_increment is correct — the composite chunker restores rowsCopied from the watermark, so it is genuinely unaffected.
The README interface block now matches copier.go exactly, and the monitoring example no longer contradicts the warning above it.
copiertest.Stub's new GetThrottler is what lets move's Status() render throttled=false without reaching the embedded nil interface.

This review was generated by Claude Code (claude-opus-5).

…blished Progress and Status now agree in the defensive branch where the chunker exists but the copier does not: both read the settled rows from the chunker and an ETA that is not yet measured. The migrate guide's resume caveat moves to the percentage row it describes and states that a resumed copy finishes short of 100%. The copier README gains the ChunkSize method entry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

aparajon · 2026-09-07T16:23:52Z

🤖 Thanks for the third pass. Addressed in 1fc647e, per finding:

API and log block disagree in the no-copier window — fixed on the Progress() side as suggested. Datasync Progress() now reports ETAMeasuring and a Summary of <n/m pct%> copyRows ETA TBD when the chunker is published but the copier is not, matching Status(). The test asserts both in that window. The comment on the guard also no longer says a tick "can land with either missing": all three runners enter CopyRows through status.Do(CopyRows, r.copier.Run), so the copier exists whenever the state is CopyRows, and the guards are defensive.
Resume caveat in the wrong row — moved to the % row, which no longer says the percentage "can drift slightly". It now states that after a resume on an auto_increment key the count restarts at the rows settled since the restart against the whole-table estimate, so % restarts low and finishes short of 100% while eta continues.
Datasync inlines what migrate/move extracted — declined again, with a sharper reason. The shared extraction the runner-triplet rule asks for already exists and all three runners use it: status.TablesFromChunker and status.CopyFromTables in pkg/status. The runner-local copyTables() in migrate and move exists only to own chunkerMu around the chunker read; datasync has no lock to own there because the chunker is read as part of the single progMu snapshot that also takes the copier, replication client, and applier. A datasync copyTables() would be a wrapper around two shared calls with nothing of its own.
Nil-copier guard added to one runner only — declined. As in 1, none of the three runners can be in CopyRows without a copier, since entering the state dereferences it. Datasync's guards predate this PR; porting a guard against an unreachable state to migrate and move would add two branches nothing can take.
README ChunkSize bullet missing — added, describing it as the chunk-size= field of the status block and why it moves during a copy.

Claude (Fable 5)

morgo

🤖 Review posted by Morgan's AI agent.

Approving at 1fc647ee. This clears the hold I put on 8f5b9a28, and it took the better of the two exits I offered.

Progress.Copy is now status.CopyFromTables(tables) summed from the same per-table snapshot that fills Progress.Tables, in all three producers. That isn't just a matching source — tables is read once and both fields are derived from it, so Copy and Tables can't disagree about the numbers or about the instant. The divergence I demonstrated (Copy: 500000/100000000 beside Tables[0]: 500000/1000000 in one snapshot) is now unconstructible rather than merely unlikely.

The new TestProgressCopyReconcilesWithTablesOnAutoIncrementKey is aimed squarely at the gap that let the original slip through: 500 contiguous ids plus one row at 1000000, on the single-auto_increment path that routes to chunkerOptimistic — the default for most production tables and the one case the old suite never touched, because its only cross-field assertion used a composite key, where Progress() and RowsCopied() read the same field and so cannot disagree. Asserting Copy.RowsCopied == 500 against a keyspace position of ~1000 is exactly the discriminating case.

The residual is documented honestly rather than papered over. The ETA is still paced on keyspace, so over a sparse range the row count and the countdown can point different directions, and the doc comment says so instead of implying they agree. copier.CopyProgress() keeps its old meaning with a comment that now names it — keyspace distance against the auto_increment max, the measure the ETA is derived from — and points callers reporting rows at status.CopyFromTables. Leaving the copier's own measure alone and fixing the layer that publishes it is the right seam.

The smaller notes went too: Copy keeps its final reading past the copy phase instead of zeroing at the boundary while Tables retained its totals, copyTables() takes one chunker read under RLock rather than three lock acquisitions per call, the test stub is shared out of copiertest, and ETA.String() makes the %v-on-a-Stringer implicit call explicit — GetETA() is now that method rather than a second copy of the same switch.

One thing worth being aware of on merge, not a blocker: Summary is no longer byte-identical to before. The format string is unchanged, but the values it renders now come from the row-count path, so on a sparse auto_increment table the copy line will read something like 500/1000 50.00% where it used to read 1000/1000000 0.10%. That is the correct number to show a human, and it's the unavoidable consequence of the fix — but it does mean the percentage in migration logs and in anything rendering it downstream changes semantics with this merge, and the percentage can now visibly lead the ETA. Worth a line in the PR body or release notes.

TablesFromChunker is nil-safe (else if chunker != nil), so a Progress() before setup yields an empty slice and a zero CopyProgress rather than a panic. 19/19 checks green.

Kiran01bm · 2026-09-08T22:47:15Z

🤖 Review findings - created by Kiran's code review agent - for spirit/pull/1220, 1fc647e.
Verdict: 4 findings — 3 non-blocking (resume % regression, 2 doc inaccuracies), 1 suggestion (no test on the resume path).

Non-blocking

The copier % now restarts at ~0 on a checkpoint resume for auto_increment PKs, discarding continuity the optimistic chunker was built to preserve. runner.go:1994 switches the copier status row and Progress().Copy from the copier's keyspace measure to CopyFromTables → RowsCopied(), but OpenAtWatermark → open() does t.actualRowsCopied.Store(0) and seeds only the keyspace counter (chunker_optimistic.go:371). Kill a 60%-copied migration and resume: the row prints copier 0.00% 0/11040 and tops out near 40% at cutover, where pre-PR it resumed at ~60%; Progress().Summary and move/runner.go:1539 regress identically. The composite chunker restores RowsCopied from its watermark (chunker_composite.go:301), so the same remedy was available for the fast path and was not applied.

The rewritten eta doc row describes the ETA as key-range distance unconditionally, which is false for composite / non-auto_increment PKs. docs/migrate.md:675 tells users an ETA/% mismatch is expected, but chunker_composite.go:446 feeds etaEstimate the identical rows/estimate ratio the % column shows — the two cannot diverge there, so a mismatch would signal a real problem. The % row above it correctly scopes its caveat to "a table with an auto_increment key"; the eta row should too.

The doc names resume as the only reason % finishes short of 100%, but the settled-row counter also under-counts on any busy table. The copy path is INSERT IGNORE, and rows the binlog applier already wrote are not reported as affected — the repo concedes this at chunk.go:35, yet chunker_optimistic.go:425 still does t.actualRowsCopied.Add(actualRows) unconditionally. So % finishes short on a fresh, non-resumed run too — a second cause docs/migrate.md:672 and the Progress.Copy doc comment don't mention.

General suggestions

The new tests only cover fresh copies, so the documented resume behaviour is asserted nowhere. progress_copy_test.go:382 never checkpoints, and resume_test.go's last copier-row assertion (line 219) runs before resumeFromCheckpoint. Dropping t.actualRowsCopied.Store(0) from open() would silently flip the documented reading with the suite green; TestCheckpoint already has the machinery to assert across the cycle.

The one thing that could have broken, verified

The new locking around the status snapshot. GetETA() no longer takes c.Lock() and delegates to GetETAState(), which does — checked both call sites (move/runner.go:1550, migration/runner.go:2005) and neither holds the copier mutex across the call, so no self-deadlock. The new chunkerMu.RLock in Status() is likewise safe: the write lock is held only across two assignments in initChunkers (runner.go:1822-1825) and the move equivalents, and Status()/Progress() are never called under it.

Verified correct

ETA.String() covers all four ETAState constants plus a fallthrough; ETANone renders 0s and is unreachable in rendered summaries.
datasync publishes copyChunker before copier, so guarding on chunker != nil strictly widens the window — it cannot hide a row that used to render.
status.CopyFromTables derives from the same tables slice as Progress.Tables in all three runners, so Copy and Tables reconcile within a snapshot.
No production code branches on Progress().Copy, Progress().ETA state, or CopyProgress()/GetProgress() — the measure change is display-only.
multiChunker keys children by QualifiedName(), so per-table summing cannot collapse same-named tables across sources in an N:M move.
fraction() guards the divide, so a 0 EstimatedRows before SetInfo yields 0.00%, not NaN.

This review was generated by Claude Code (claude-opus-5).

The optimistic chunker now writes its watermark in the same envelope the composite chunker uses, carrying the rows settled beside the resume position, and restores that count on OpenAtWatermark. A resumed run therefore reports the copy where the previous run left it instead of restarting at zero, in Progress and in the status log block. Bare chunk watermarks from older checkpoints are still accepted and resume with a zero count, as before. The docs replace the resume caveat with the one that remains: rows the binlog applier wrote ahead of the copy are not counted, since INSERT IGNORE reports them as unaffected, so a busy table finishes short of its estimate. The eta row is scoped to auto_increment keys, where it is keyspace paced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Kiran01bm · 2026-09-09T00:07:14Z

🤖 Review findings - created by Kiran's code review agent - for spirit/pull/1220, 8003a8e.
Re-review of the delta only: 1fc647ee..8003a8e6 (the range since the previous review comment). Findings from that earlier comment are not repeated here.
Verdict: 3 findings — 0 blocking, 3 non-blocking (move-resume double count, two stale contract docs).

Non-blocking

Move resume double-counts the recopy range, so the new "count does not double up across the resume" claim does not hold for spirit move. move/runner.go#L2381 deleteRecopyRange DELETEs every target row at/above the watermark lower bound before chunker_optimistic.go#L377 restores the full pre-crash total — which provably includes chunks settled above the watermark. The re-copy's INSERT IGNORE therefore reports those rows as affected and actualRowsCopied.Add counts them a second time, over-reporting by up to (in-flight chunks + 1) × chunk-size. Migration is safe only because its rows are still present (IGNORE reports 0), which is exactly what the new comment in resume_test.go#L273 asserts; impact is reporting-only, and no move test covers the deleted-then-recopied case.

The Chunker.RowsCopied interface contract is now inverted and was missed by the doc sweep. chunker.go#L76 still reads "the optimistic chunker's watermark stores key positions only", while the same commit made GetLowWatermark marshal watermarkEnvelope{ChunkJSON, RowsCopied} and OpenAtWatermark call actualRowsCopied.Store(settled). chunker_multi.go#L216 points readers at this very doc "for resume semantics", so anyone implementing a new chunker is told the count restarts at zero. The PR swept docs/migrate.md, pkg/status/README.md, progress.go and chunk.go but chunker.go is absent from its 12 changed files.

The per-invocation RecordCopyCompleted contract is now false at four emission sites. Rows now span invocations (restored from the checkpoint at runner.go#L286) while chunks still restarts at zero, since chunksCopied is cleared only in Reset(). The comment three lines above at migration/runner.go#L277 — repeated verbatim at move/runner.go#L239, datasync/runner.go#L186, status/tracker.go#L197 and status/README.md#L39 — still says a resumed invocation reports only post-resume work, contradicting the caveat this same commit rewrote in progress.go. A wrapper summing the per-attempt aggregate now reports 3M rows for a 2M-row table, and rows/chunks in one callback are no longer commensurable.

The one thing that could have broken, verified

Backward compatibility of pre-PR checkpoints through unwrapWatermark. A bare chunk JSON unmarshals into watermarkEnvelope without error (unknown keys ignored) leaving ChunkJSON == "", so the guard err == nil && envelope.ChunkJSON != "" falls through to return watermark, 0 and the legacy payload is passed on untouched. Confirmed every persisted copier watermark consumer routes through OpenAtWatermark or WatermarkPerTable — migration/runner.go#L1686, move/runner.go#L590, datasync/runner.go#L1228, checksum/single.go#L734, checksum/distributed.go#L733 — so no caller receives an envelope where it expected bare chunk JSON.

Verified correct

WatermarkPerTable's multi-map branch still cannot swallow the envelope: RowsCopied is a JSON number, so the map[string]string unmarshal errors and the single-table branch is reached (chunk.go#L362).
OpenAtWatermark ordering is right: open() zeroes actualRowsCopied at line 709 (called at line 306), well before the restore at line 377.
move.deleteRecopyRange reaches WatermarkRecopyClause/newChunkFromJSON with unwrapped chunk JSON, so the envelope never trips the foreign-format guard.
Renaming compositeWatermark to watermarkEnvelope is behaviour-preserving — identical field names and JSON tags, so existing composite checkpoints still decode.
The "INSERT IGNORE rows are reported unaffected" claim in docs/migrate.md and progress.go is accurate: single_target.go#L569 and sharded.go#L603 both use INSERT IGNORE, fed to Feedback at buffered.go#L114.
The restored count cannot corrupt ETA or percentage: getCopyStats and estimateRowsPerSecondLoop read chunker.Progress() (keyspace distance), never RowsCopied(), so no spurious copy-rate sample at resume.

This review was generated by Claude Code (claude-opus-5).

aparajon · 2026-09-09T01:03:45Z

🤖 Thanks for the fourth pass. Addressed in 8003a8e, per finding:

Copier % restarts at ~0 on a checkpoint resume for auto_increment PKs — fixed, and you were right that the composite remedy carried over. The optimistic chunker's watermark is now the same {ChunkJSON, RowsCopied} envelope the composite chunker writes, with RowsCopied the settled count, and OpenAtWatermark restores it after open() zeroes the counter. Bare chunk watermarks from older checkpoints still load and resume with a zero count, as before. The shared parsers (WatermarkPerTable, and through it the move recopy clause) already unwrapped the envelope regardless of which chunker wrote it, so the only consumer that needed a change was a move test decoding the stored watermark directly.
eta doc row describes keyspace pacing unconditionally — fixed. The row now scopes the key-range measure to auto_increment keys and says that on other keys the ETA is paced on the same rows-against-estimate ratio as %, so the two move together there.
% finishes short of 100% on any busy table, not only after a resume — fixed in the % row, the Progress.Copy doc comment, and the status README: rows the binlog applier wrote before the copy reached them are not counted because INSERT IGNORE reports them as unaffected. The resume caveat is gone from all three, since a resume now continues the count.
Resume behaviour asserted nowhere — TestCheckpoint now covers the cycle: the watermark's RowsCopied equals the rows settled before the checkpoint; right after resumeFromCheckpoint, Progress().Copy.RowsCopied and the status copier row report that same count before anything is copied; re-copying the watermark chunk, whose rows are already in the new table, leaves the count unchanged; and the ten chunks after it raise it. TestOptimisticResumeProgressAccounting additionally pins that a bare chunk watermark resumes with a zero count.

Claude (Fable 5)

…esume The chunker now restores its settled row count from the checkpoint so that progress continues across a resume, which would have made the copy aggregate reported to the metrics sink span invocations while its chunk count did not. Each runner records the count restored at resume and subtracts it when the copy completes, so the aggregate keeps reporting only the rows and chunks the invocation settled itself. The checkpoint test asserts the reported figures after a resume. The chunker contract and the status docs describe the restored count, and name the move resume, which deletes and re-copies the rows at or above the resume position and so counts the previously settled ones among them again. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

aparajon · 2026-09-09T01:18:29Z

🤖 Thanks for the fifth pass. Addressed in 8897b76, per finding:

Move resume double-counts the recopy range — confirmed, and documented rather than fixed here. deleteRecopyRange removes the target rows at or above the resume position, so the ones among them that were settled before the checkpoint are re-copied as affected rows and counted twice; the composite chunker has behaved this way in spirit move since it began restoring RowsCopied from its watermark, and this PR extends it to the optimistic chunker. An exact correction needs the chunker to learn how many rows the recopy delete removed, which is a new Chunker method on a public interface, so I have left it for a follow-up and stated the behaviour on Progress.Copy, in the status README, and in the PR body. The resume test comment now scopes its no-double-count claim to migrations, where the new table keeps the re-copied rows.
Chunker.RowsCopied contract inverted — fixed. The interface doc now says the watermark carries the count beside the position on both chunkers, so a resumed run continues from the previous run's settled rows, and that a bare chunk watermark from before the count was recorded resumes at zero.
Per-invocation RecordCopyCompleted contract false at four sites — kept the contract true instead of rewriting it. Each runner records copyChunker.RowsCopied() right after OpenAtWatermark and subtracts it in recordCopyCompleted, so the aggregate reports only the rows this invocation settled, alongside the chunks it copied. The comments at the three runner sites and the status README describe the subtraction. TestCheckpoint now installs a metrics sink after the resume and asserts the reported rows equal the chunker's count minus the restored one, and the chunks equal the eleven that runner copied.

Claude (Fable 5)

Kiran01bm · 2026-09-09T02:03:08Z

🤖 Review findings - created by Kiran's code review agent - for spirit/pull/1220, 8897b76.
Re-review of the delta only: 8003a8e6..8897b76f (the range since the previous review comment). Findings from that earlier comment are not repeated here.

Verdict: 3 findings — 1 blocking (stale copyRowsAtResume underflows the copy aggregate), 2 non-blocking test gaps.

Blocking

copyRowsAtResume is never cleared when a resume falls back to a fresh migration — the copy aggregate under-reports and can underflow uint64. runner.go:1691 sets the field right after OpenAtWatermark(), but the definitive status.ErrBinlogNotFound return at runner.go:1726 sends setup() down the newMigration() path (runner.go:1359), which rebuilds a zero-based chunker at runner.go:1826 while the stale count survives. Since runCopy fires defer r.recordCopyCompleted() even on failure/cancel, the unconditional subtraction at runner.go:288 wraps: a checkpoint of 60M rows plus a fresh copy cancelled at 10M emits float64(1.8446744e19) to the CopyRowsCompletedMetricName gauge and to RecordWorkflowCopyCompleted, with no clamp in Tracker.RecordCopyCompleted. Fix by zeroing r.copyRowsAtResume on the fresh-start fallback, or by making the subtraction saturating at line 288 (and at move/runner.go:250, datasync/runner.go:197).

Non-blocking

No test pins the copyRowsAtResume <= RowsCopied() invariant the subtraction depends on. resume_test.go:304 asserts require.Equal(t, r.copyChunker.RowsCopied()-settled, sink.rows) on a runner that resumed successfully, so it holds for any value of the field that happens to equal settled. A case that forces a definitive resume failure into newMigration() — or simply asserts sink.rows <= r.copyChunker.RowsCopied() — would have caught the blocking bug above.

The identical change in move and datasync has no coverage at all. Deleting r.copyRowsAtResume = r.copyChunker.RowsCopied() from move/runner.go:595 or datasync/runner.go:1233 leaves the suite green — neither package installs a metrics sink, so Tracker.RecordCopyCompleted's if !t.hasSink.Load() { return } makes the subtraction unobservable there.

General suggestions

copyAggregateSink duplicates the existing phaseSink recorder in the same package. resume_test.go:310 could have used newPhaseSink() plus get(metrics.CopyRowsCompletedMetricName) from phase_metrics_test.go, which is already compiled into the same build and already asserts that exact metric. Cosmetic only — delivery is single-goroutine and synchronous, so the missing mutex is not an actual race.

The one thing that could have broken, verified

Turning a cumulative gauge into a per-invocation one hinges on RowsCopied() being monotonic from the restored baseline. Both OpenAtWatermark implementations (chunker_optimistic.go:377, chunker_composite.go:296) restore the settled count and only Add to it in Feedback, and Chunker.Reset() — the one call that would zero actualRowsCopied — is reachable only on checksum chunkers, never on r.copyChunker. So the ordinary resume path cannot underflow; only the fresh-start fallback above does.

Verified correct

datasync/runner.go:1233 — startResume() and startFresh() are mutually exclusive, so the field cannot go stale there.
move/runner.go:595 — every definitively-unresumable error returns before the assignment, so the newCopy() fallback never sees a stale value.
chunksCopied is deliberately not restored by either OpenAtWatermark, so the chunks half is already per-invocation and correctly left unadjusted.
recordCopyCompleted() reads r.copier.GetChunker(), which is the same object as r.copyChunker in all three runners — one counter, not two.
Tracker.RecordCopyCompleted → send() is fully synchronous, so the new sink assertion reads sink.rows after delivery without a race.
table/chunk.go:395 unwrapWatermark falls back to (watermark, 0) for bare pre-envelope chunk JSON, so the new "resumes at zero" doc claim is accurate and backward compatible.

This review was generated by Claude Code (claude-opus-5).

morgo

🤖 Review posted by Morgan's AI agent.

Flagging, not blocking — the approval standing on this PR predates the last two commits (8003a8e6, 8897b76f), which are a bigger change than the rest of the PR: the optimistic chunker's persisted watermark goes from bare chunk JSON to the {"ChunkJSON":…,"RowsCopied":N} envelope. That's the checkpoint format for every single-table auto-inc migration, i.e. the common case. 19/19 green.

The direction is right and the compatibility work is real: unwrapWatermark accepts the bare form and returns a zero count, TestOptimisticResumeProgressAccounting pins that, and the composite chunker already used this exact envelope so the two are now one type instead of two. One bug and one thing to write down.

copyRowsAtResume survives a resume that gives up and starts fresh, and the subtraction underflows.

migration/runner.go:1691 and move/runner.go:595 set copyRowsAtResume mid-resume, but both files have a path where the resume then fails definitively and the run continues on the fresh path with a brand-new chunker:

migration: resumeFromCheckpoint gets ErrBinlogNotFound from StartFromPosition (purged binlogs — exactly the case that classification exists for), so setup falls through to newMigration at runner.go:1359. newMigration calls initChunkers, which reassigns r.copyChunker to fresh chunkers whose actualRowsCopied is 0.
move: resumeFromCheckpoint fails, --force is set and isDefinitivelyUnresumable, so runner.go:701 takes the wipe-and-start-fresh path.

copyRowsAtResume still holds the old checkpoint's count. recordCopyCompleted then computes chunker.RowsCopied() - r.copyRowsAtResume on uint64. runCopy calls it from a defer, so it fires when the copy fails or is cancelled too — and a fresh copy that stops before it settles as many rows as the discarded checkpoint had underflows. Concretely: checkpoint at 5M rows, binlogs purged, fresh migration starts, operator Ctrl-Cs at 1M → copy-rows-completed is emitted as ~1.8e19. On a fresh run that does finish, it doesn't underflow, it just under-reports by the discarded count.

Fix is one line on each fallback path — r.copyRowsAtResume = 0 next to r.checksumWatermark = "" at move/runner.go:701, and before r.newMigration(ctx) at migration/runner.go:1359. That checksumWatermark clear is the same hazard, and its comment already argues for exactly this discipline: "Clear it explicitly before the fresh path so a future force-eligible failure added after that boundary cannot leak stale checkpoint state into newCopy." This is the future failure that comment anticipated.

datasync is fine — startResume at runner.go:732 returns directly, with no fall-through to startFresh.

Downgrade is a one-way door and isn't stated anywhere. A spirit built before this PR reading a checkpoint written after it hits newChunkFromJSON's shape validation, which rejects the envelope by design. That error carries no sentinel, so resumeErrorIsDefinitive returns false and setup refuses to start fresh. Fails safe — nothing is dropped, no corruption, the _new table and checkpoint are preserved — but a rollback strands every in-flight migration until the operator either rolls forward or drops the checkpoint table. Both persisted watermarks are affected (ChecksumWatermark too, since the checksum chunker is the same type). Worth a line in the PR description or release notes so whoever does the rollback isn't diagnosing it live.

The move double-count is disclosed and I agree with the call. deleteRecopyRange runs before OpenAtWatermark, so the rows it deletes get re-copied and counted a second time on top of the restored count that already included them. progress.go and the README both say so plainly. Making the number exact would mean the checkpoint carrying a per-range breakdown; not worth it for a progress figure, and stating the skew is the better trade.

Nit: recordCopyCompleted is now duplicated verbatim across all three runners, including the comment. It only touches r.copier, r.status and r.copyRowsAtResume — a small shared helper taking those three would keep the next correction from having to land in three places.

aparajon mentioned this pull request Sep 6, 2026

refactor: render Spirit progress from structured fields, not a parsed string block/schemabot#1314

Draft

aparajon marked this pull request as ready for review September 6, 2026 23:04

morgo reviewed Sep 6, 2026

View reviewed changes

morgo approved these changes Sep 8, 2026

View reviewed changes

morgo reviewed Sep 9, 2026

View reviewed changes

Uh oh!

Conversation

aparajon commented Sep 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What

Uh oh!

morgo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kiran01bm commented Sep 7, 2026

Non-blocking

General suggestions

The one thing that could have broken, verified

Verified correct

Uh oh!

aparajon commented Sep 7, 2026

Uh oh!

Kiran01bm commented Sep 7, 2026

Non-blocking

General suggestions

The one thing that could have broken, verified

Verified correct

Uh oh!

aparajon commented Sep 7, 2026

Uh oh!

Kiran01bm commented Sep 7, 2026

Non-blocking

General suggestions

The one thing that could have broken, verified

Verified correct

Uh oh!

aparajon commented Sep 7, 2026

Uh oh!

morgo left a comment

Choose a reason for hiding this comment

Uh oh!

Kiran01bm commented Sep 8, 2026

Non-blocking

General suggestions

The one thing that could have broken, verified

Verified correct

Uh oh!

Kiran01bm commented Sep 9, 2026

Non-blocking

The one thing that could have broken, verified

Verified correct

Uh oh!

aparajon commented Sep 9, 2026

Uh oh!

aparajon commented Sep 9, 2026

Uh oh!

Kiran01bm commented Sep 9, 2026

Blocking

Non-blocking

General suggestions

The one thing that could have broken, verified

Verified correct

Uh oh!

morgo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aparajon commented Sep 6, 2026 •

edited

Loading